Skip to content

Fix serialization when using interface as resource#2822

Merged
teohhanhui merged 1 commit into
api-platform:2.4from
teohhanhui:fix/serializer-readable-writable-link
Jun 8, 2019
Merged

Fix serialization when using interface as resource#2822
teohhanhui merged 1 commit into
api-platform:2.4from
teohhanhui:fix/serializer-readable-writable-link

Conversation

@teohhanhui

@teohhanhui teohhanhui commented May 25, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor
Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets N/A
License MIT
Doc PR N/A

TODO:

  • Add tests

teohhanhui
teohhanhui commented May 25, 2019
View reviewed changes
Comment thread src/Metadata/Property/Factory/SerializerPropertyMetadataFactory.php Outdated
$propertyMetadata = $this->transformLinkStatus($propertyMetadata, $normalizationGroups, $denormalizationGroups);
} catch (ResourceClassNotFoundException $e) {
// No need to check link status if related class is not a resource
// hack to allow non-resource classes (actually they should not be supported)

@teohhanhui teohhanhui May 25, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this hack is removed, all hell breaks loose, because we're using the PropertyMetadataFactory for input/output classes (DTOs). 😞

@dunglas dunglas May 25, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we done it on purpose. It's a good move actually, it allows to retrieve the shape of any PHP structure, regardless if it is mapped with @ApiResource or not.

@teohhanhui teohhanhui May 25, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I'll update the comment. 😎

@teohhanhui teohhanhui May 26, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But actually, I can see many problems with using the property metadata system like this. It's simply not designed for non-resources, as you can see here. It'd give the wrong results (as in this case), which is arguably worse than not using it at all.

@teohhanhui teohhanhui May 27, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dunglas I hope this is okay for now: https://github.com/api-platform/core/compare/075222cd357ad018bc517e954ab46b5b4e9e90b3..731b45f5554d7ec832a33ed59fd1d1fac2230373

@teohhanhui
teohhanhui force-pushed the fix/serializer-readable-writable-link branch from 1071f97 to 075222c Compare May 25, 2019 20:06
@teohhanhui teohhanhui changed the title Fix SerializerPropertyMetadataFactory readable/writable link value for resource with inheritance Fix SerializerPropertyMetadataFactory readable/writable link status for resource with inheritance May 25, 2019
@teohhanhui
teohhanhui force-pushed the fix/serializer-readable-writable-link branch from 075222c to 731b45f Compare May 27, 2019 13:23
@teohhanhui teohhanhui mentioned this pull request May 27, 2019
Merged
@teohhanhui
teohhanhui force-pushed the fix/serializer-readable-writable-link branch from 731b45f to a3fd33c Compare May 28, 2019 20:31
@teohhanhui teohhanhui changed the title Fix SerializerPropertyMetadataFactory readable/writable link status for resource with inheritance Fix serialization when using interface as resource May 28, 2019
@teohhanhui
teohhanhui force-pushed the fix/serializer-readable-writable-link branch 3 times, most recently from 8330e42 to 8b5b129 Compare May 28, 2019 21:03
teohhanhui
teohhanhui commented May 28, 2019
View reviewed changes
Comment thread composer.json Outdated
@teohhanhui
teohhanhui force-pushed the fix/serializer-readable-writable-link branch from 8b5b129 to f8e3011 Compare May 28, 2019 21:09
@teohhanhui
teohhanhui force-pushed the fix/serializer-readable-writable-link branch from f8e3011 to 50221f5 Compare May 29, 2019 06:29
@teohhanhui
teohhanhui requested a review from alanpoulain May 29, 2019 06:42
alanpoulain
alanpoulain reviewed Jun 4, 2019
View reviewed changes
Comment thread features/bootstrap/DoctrineContext.php Outdated
alanpoulain
alanpoulain reviewed Jun 4, 2019
View reviewed changes
Comment thread src/Hydra/Serializer/DocumentationNormalizer.php
alanpoulain
alanpoulain reviewed Jun 4, 2019
View reviewed changes
Comment thread src/Metadata/Property/Factory/SerializerPropertyMetadataFactory.php
$this->resourceMetadataFactory = $resourceMetadataFactory;
$this->serializerClassMetadataFactory = $serializerClassMetadataFactory;
$this->decorated = $decorated;
$this->resourceClassResolver = $resourceClassResolver;

@alanpoulain alanpoulain Jun 4, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deprecated if null?

@teohhanhui teohhanhui Jun 4, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... We need to do this in many places. Will do this in a follow-up PR?

@teohhanhui
teohhanhui force-pushed the fix/serializer-readable-writable-link branch from 50221f5 to 7bdb0f6 Compare June 4, 2019 16:03
@teohhanhui

teohhanhui commented Jun 4, 2019

Copy link
Copy Markdown
Contributor Author

Build failures should be fixed by #2841

We shall re-trigger the build after that's merged.

@teohhanhui
teohhanhui force-pushed the fix/serializer-readable-writable-link branch from 7bdb0f6 to 6a64d36 Compare June 5, 2019 12:47
@teohhanhui

teohhanhui commented Jun 5, 2019

Copy link
Copy Markdown
Contributor Author

Build still failing, will be fixed by #2843

@teohhanhui
teohhanhui force-pushed the fix/serializer-readable-writable-link branch from 6a64d36 to bba7e32 Compare June 7, 2019 14:45
@teohhanhui
teohhanhui force-pushed the fix/serializer-readable-writable-link branch from bba7e32 to c9d9c58 Compare June 7, 2019 16:04
@teohhanhui
teohhanhui merged commit 9044d7f into api-platform:2.4 Jun 8, 2019
@soyuka

soyuka commented Jun 10, 2019

Copy link
Copy Markdown
Member

nice :D @Nek- will be happy with this :D

@teohhanhui
teohhanhui deleted the fix/serializer-readable-writable-link branch June 12, 2019 07:50
@bendavies

bendavies commented Jun 12, 2019

Copy link
Copy Markdown
Contributor

this has caused a regression for me and I can't seem to fix

{"type":"https:\/\/tools.ietf.org\/html\/rfc2616#section-10","title":"An error occurred","detail":"Object of type \u0022ClassA\u0022 does not match \u0022ClassB\u0022 resource class."}

I have an operation which returns a different class to resources the operation is defined on.

@teohhanhui

teohhanhui commented Jun 12, 2019

Copy link
Copy Markdown
Contributor Author

@bendavies Can you open an issue?

@bendavies bendavies mentioned this pull request Jun 12, 2019
Closed
@bendavies

bendavies commented Jun 12, 2019

Copy link
Copy Markdown
Contributor

@teohhanhui done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alanpoulain alanpoulain alanpoulain left review comments
@dunglas dunglas dunglas approved these changes
@soyuka soyuka soyuka approved these changes
@meyerbaptiste meyerbaptiste Awaiting requested review from meyerbaptiste

Assignees

No one assigned

Labels

[RFR] Ready for Review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@teohhanhui @soyuka @bendavies @dunglas @alanpoulain